[GH-2830] Add Geography dual-dispatch to ST_AsText#2849
Conversation
- L1 — JTS delegation via getJTSGeometry().toText() - 1 new Spark SQL test - update document
There was a problem hiding this comment.
Pull request overview
Adds Geography support to the existing ST_AsText Spark SQL expression via dual-dispatch, so it returns WKT for both Geometry and Geography inputs (Geography path delegates to common.geography.Functions.asText).
Changes:
- Added Geography overload wiring for
ST_AsTextin Spark SQL expressions. - Added
Functions.asText(Geography)implementation incommonGeography functions. - Added/updated docs and added new unit/integration tests for
ST_AsTexton Geography.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala | Dual-dispatches ST_AsText to Geometry (asWKT) or Geography (common.geography.Functions.asText). |
| common/src/main/java/org/apache/sedona/common/geography/Functions.java | Implements asText(Geography) by converting to JTS and calling toText(). |
| common/src/test/java/org/apache/sedona/common/Geography/FunctionTest.java | Adds unit tests for Functions.asText (point, polygon, null). |
| spark/common/src/test/scala/org/apache/sedona/sql/geography/GeographyFunctionTest.scala | Adds Spark SQL integration test for ST_AsText on Geography. |
| docs/api/sql/geography/Geography-Functions/ST_AsText.md | Adds Geography documentation page for ST_AsText. |
| docs/api/sql/geography/Geography-Functions.md | Adds ST_AsText entry to the Geography functions index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Return the WKT text representation of a geography. */ | ||
| public static String asText(Geography g) { | ||
| if (g == null) return null; | ||
| return toJTS(g).toText(); |
There was a problem hiding this comment.
@zhangfengcdt This toJTS function seem to SerDe WKB one more time. Is it possible to avoid it?
There was a problem hiding this comment.
Good question! toJTS(g) on WKBGeography does one WKB → JTS parse and caches the result in jtsGeometry (WKBGeography.java:116). Subsequent calls — both later asText invocations on the same row and any other JTS-path function (ST_NPoints, ST_GeometryType, ST_Centroid, ST_NumGeometries) — reuse the cached Geometry for free.
The alternative — g.toString() on the Geography — actually goes through WKBGeography.toString() getS2Geography().toString(), which populates the other cache (s2Geography), so it's worse when the current implementation.
A true zero-parse path (WKB → WKT walker) would need custom code since JTS doesn't expose it.
Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-XXX] my subject. Closes #<issue_number>What changes were proposed in this PR?
How was this patch tested?
Did this PR include necessary documentation updates?